Skip to content

Refactor NotificationManager::Notification - #2347

Open
DavisNT wants to merge 1 commit into
InfiniTimeOrg:mainfrom
DavisNT:notifrefactor
Open

Refactor NotificationManager::Notification#2347
DavisNT wants to merge 1 commit into
InfiniTimeOrg:mainfrom
DavisNT:notifrefactor

Conversation

@DavisNT

@DavisNT DavisNT commented Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Refactor NotificationManager::Notification to use constructors. This reduces risk of coding errors (incl. buffer overflows) when creating NotificationManager::Notification and copying text to it.

Additionally fix a latent bug in ImmediateAlertService (include null terminator in the bytes copied and properly set size) using the constructor.

This PR supersedes #2159

This PR needs corresponding refactoring in InfiniSim: InfiniTimeOrg/InfiniSim#181

@DavisNT DavisNT mentioned this pull request Oct 2, 2025
Open
@github-actions

github-actions Bot commented Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 385184B -48B
data 944B 0B
bss 22624B -16B

Run in InfiniEmu

@DavisNT DavisNT mentioned this pull request Oct 2, 2025
Merged
@DavisNT

DavisNT commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

@mark9064, @NeroBurner Here is refactoring of NotificationManager::Notification with constructor. Can you please review this?

P.S. InfiniSim doesn't build, because it needs InfiniTimeOrg/InfiniSim#181

@mark9064 mark9064 added the maintenance Background work label Oct 2, 2025
@NeroBurner NeroBurner added this to the 1.17.0 milestone Nov 4, 2025
@BurninTurtles BurninTurtles mentioned this pull request May 25, 2026
Open
@DavisNT
DavisNT force-pushed the notifrefactor branch 2 times, most recently from 181276e to 37a9a18 Compare July 20, 2026 20:33
@DavisNT

DavisNT commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@mark9064 , @NeroBurner I have rebased this PR (and retested services).
Could it be possible to do the final review/verifications and merge this PR (and the corresponding InfiniSim PR InfiniTimeOrg/InfiniSim#181 )?

@DavisNT

DavisNT commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@mark9064 , @NeroBurner Could it be possible to merge this PR (and the corresponding InfiniSim PR InfiniTimeOrg/InfiniSim#181 )?

mark9064
mark9064 reviewed Aug 11, 2026
View reviewed changes

@mark9064 mark9064 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's been a while. Check the date on that first comment!

Comment thread src/components/ble/NotificationManager.cpp Outdated
Comment thread src/components/ble/NotificationManager.cpp
Comment thread src/components/ble/NotificationManager.cpp
@DavisNT
Refactor NotificationManager::Notification
f69ab9e 
Refactor NotificationManager::Notification to use constructors.
This reduces risk of coding errors (incl. buffer overflows) when
creating NotificationManager::Notification and copying text to it.
@DavisNT

DavisNT commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@mark9064 Sorry - I had somehow missed your comment about the cast style! Now it should be fixed in both places.
Docstrings are also added.
Can you please review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mark9064 mark9064 mark9064 left review comments

Assignees

No one assigned

Labels

maintenance Background work

Projects

None yet

Milestone

1.17.0

Development

Successfully merging this pull request may close these issues.

3 participants

@DavisNT @mark9064 @NeroBurner